Adds WebUI handler for Android AI #20129

SergeyZhukovsky · 2023-09-12T13:55:17Z

Resolves brave/brave-browser#32808

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

petemill

This is great but as of #20068 we no longer observe the TabStripModel and no longer need to change active / target WebContents. All platforms will now only observe 1 WebContents at a time when opened.

So I think we can simplify this - make the same AIChatUIPageHandler implementation for both platforms, and have it accept a single target WebContents in the ctor. AIChatUI can select the currently active target WebContents per-platform (L91) - tab_strip_model->GetActiveWebContents() for desktop and your specific GetActiveWebContents for android.

What do you think?

SergeyZhukovsky · 2023-09-15T01:44:56Z

This is great but as of #20068 we no longer observe the TabStripModel and no longer need to change active / target WebContents. All platforms will now only observe 1 WebContents at a time when opened.

So I think we can simplify this - make the same AIChatUIPageHandler implementation for both platforms, and have it accept a single target WebContents in the ctor. AIChatUI can select the currently active target WebContents per-platform (L91) - tab_strip_model->GetActiveWebContents() for desktop and your specific GetActiveWebContents for android.

What do you think?

ok, sounds good, going to do it that way in that case.

SergeyZhukovsky · 2023-09-15T20:22:40Z

@petemill I re-made it following the new code requirements, it's good for another review round.

petemill

Thanks for re-working. Just some minor feedback and this looks great

petemill · 2023-09-15T22:13:37Z

browser/ui/webui/ai_chat/ai_chat_ui.cc

+                      web_ui()->GetWebContents()) != TabStripModel::kNoTab;
+#else
+  web_contents = GetActiveWebContents(profile_);
+  is_standalone = web_contents == nullptr;


Shouldn't the is_standalone for both platforms be based on whether this WebUI is the active Tab vs a separate WebContents, i.e. for both platforms:

is_standalone = web_contends !== web_ui->GetWebContents()

?

yeah perhaps it's better to do that way for the feature standalone option on Android

actually that's not right as web_contents is the current page contents vs web_ui->GetWebContents() is an actual web ui with the chat contents. So I think Android detection is correct it means we have a chat opened only if we don't have an active tab and the web ui, but for desktop I'm not sure exactly what do we check there, we are looking for an index of the chat web ui and we assume it's not a standalone if we found it. As to me the check should be
is_standalone = web_contents == web_ui->GetWebContents();

petemill · 2023-09-15T22:16:23Z

browser/ui/webui/ai_chat/ai_chat_ui.h

@@ -41,7 +44,9 @@ class AIChatUI : public ui::UntrustedWebUIController {

  base::WeakPtr<ui::MojoBubbleWebUIController::Embedder> embedder_;
  raw_ptr<Profile> profile_ = nullptr;
+#if !BUILDFLAG(IS_ANDROID)
  raw_ptr<Browser> browser_ = nullptr;


I don't think we need browser_ for either platform now

yeah I noticed that also about the member, I just didn't remove as thought we kept it for some feature interaction. I will remove it.

petemill · 2023-09-15T22:20:22Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.h

                            public AIChatTabHelper::Observer,
                            public content::WebContentsObserver {
 public:
  AIChatUIPageHandler(
      content::WebContents* owner_web_contents,
-      TabStripModel* tab_strip_model,
+      bool is_standalone,
+      content::WebContents* web_contents,


nit: perhaps a more specific name, e.g. chat_context_web_contents?

petemill · 2023-09-15T22:20:53Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.h

                            public AIChatTabHelper::Observer,
                            public content::WebContentsObserver {
 public:
  AIChatUIPageHandler(
      content::WebContents* owner_web_contents,
-      TabStripModel* tab_strip_model,
+      bool is_standalone,


I wonder if we even need is_standalone? Perhaps consumer just passes nullptr for the WebContents param?

yeah it could be detected that way, I will change it

SergeyZhukovsky · 2023-09-18T19:35:56Z

@petemill it's good for another review round.

petemill

👍

petemill · 2023-09-20T16:23:24Z

browser/ui/webui/ai_chat/ai_chat_ui_page_handler.cc

-    bool is_visible =
-        (web_contents->GetVisibility() == content::Visibility::VISIBLE) ? true
-                                                                        : false;
+    bool is_visible = (chat_context_web_contents->GetVisibility() ==


That's strange, this is meant to check visibility of owner_web_contents. I guess it's an existing bug, but if you have to make any other changes to this PR then please change it. But it's fine to leave it and we'll fix it separately.

thypon

security gtg

SergeyZhukovsky requested review from petemill and nullhook September 12, 2023 13:55

SergeyZhukovsky self-assigned this Sep 12, 2023

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Sep 12, 2023

petemill requested changes Sep 14, 2023

View reviewed changes

SergeyZhukovsky force-pushed the android_ai_handler branch from f8a296f to dc3c53b Compare September 15, 2023 19:18

petemill requested changes Sep 15, 2023

View reviewed changes

SergeyZhukovsky force-pushed the android_ai_handler branch from dc3c53b to a293c5b Compare September 18, 2023 18:44

Adds WebUI handler for Android AI

e897f71

SergeyZhukovsky force-pushed the android_ai_handler branch from a293c5b to e897f71 Compare September 20, 2023 15:00

petemill approved these changes Sep 20, 2023

View reviewed changes

SergeyZhukovsky merged commit 0d8d5f1 into master Sep 20, 2023
6 checks passed

SergeyZhukovsky deleted the android_ai_handler branch September 20, 2023 18:49

github-actions bot added this to the 1.60.x - Nightly milestone Sep 20, 2023

thypon reviewed Jan 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds WebUI handler for Android AI #20129

Adds WebUI handler for Android AI #20129

SergeyZhukovsky commented Sep 12, 2023 •

edited

Loading

petemill left a comment

SergeyZhukovsky commented Sep 15, 2023

SergeyZhukovsky commented Sep 15, 2023

petemill left a comment

petemill Sep 15, 2023

SergeyZhukovsky Sep 15, 2023

SergeyZhukovsky Sep 18, 2023 •

edited

Loading

petemill Sep 15, 2023

SergeyZhukovsky Sep 15, 2023

petemill Sep 15, 2023

petemill Sep 15, 2023

SergeyZhukovsky Sep 15, 2023

SergeyZhukovsky commented Sep 18, 2023

petemill left a comment

petemill Sep 20, 2023

thypon left a comment

Adds WebUI handler for Android AI #20129

Adds WebUI handler for Android AI #20129

Conversation

SergeyZhukovsky commented Sep 12, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

petemill left a comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 15, 2023

SergeyZhukovsky commented Sep 15, 2023

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 18, 2023

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thypon left a comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 12, 2023 •

edited

Loading

SergeyZhukovsky Sep 18, 2023 •

edited

Loading